Fix phpstan/phpstan#14458: Regression: never defined variable#5451
Conversation
…as never defined - Skip No-certainty conditional expressions in Pass 2 (supertype match) of filterBySpecifiedTypes - The supertype match could incorrectly resolve only the "undefined" branch of a conditional variable, marking it as never defined - New regression test in tests/PHPStan/Rules/Variables/data/bug-14458.php
|
|
||
| // Pass 2: Supertype match. Only runs when Pass 1 found no exact match for this expression. | ||
| foreach ($conditionalExpressions as $conditionalExpression) { | ||
| if ($conditionalExpression->getTypeHolder()->getCertainty()->no()) { |
There was a problem hiding this comment.
do we need a similar check for the "Pass 1" foreach loop a few lines above?
if so, add a test-case which fails without a similar fix for "Pass 1".
There was a problem hiding this comment.
Pushed. Here's a summary of the review response:
staabm's question: Does Pass 1 need a similar No-certainty check?
Answer: No. The key difference is the matching strategy:
-
Pass 1 uses exact matching (
equals()). If the specified type exactly equals the No branch's condition type, the scope genuinely corresponds to the "variable is undefined" state. Exact matching can't produce the asymmetry that causes the bug — it's impossible for a type to simultaneously equal two different condition types. -
Pass 2 uses supertype matching (
isSuperTypeOf()), which is inherently asymmetric. A broader condition type (the "undefined" branch, with fewer constraints likearray&hasOffsetValue('a', 'b')) is more likely to be a supertype of a narrowed type than a narrower condition type (the "defined" branch, likearray&hasOffsetValue('a', 'b')&hasOffsetValue('c', ...)). This asymmetry causes only the No-certainty branch to match, incorrectly resolving the variable as never defined.
I added a comment explaining this reasoning directly in the code at the Pass 2 check, so future readers understand why the guard is only needed there. All tests (11,853) and make phpstan pass.
Address review feedback: Pass 1 uses exact matching which cannot produce the asymmetry that causes the bug in Pass 2. The supertype match in Pass 2 is inherently asymmetric - a broader condition type (the "undefined" branch) is more likely to be a supertype of a narrowed type than a narrower condition type (the "defined" branch). Exact matching has no such asymmetry. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…check" This reverts commit 19ec231.
|
Thank you. |
4 participants
Summary
After #5386 (commit 52704a4), a variable conditionally defined inside an
if (isset(...))block was incorrectly reported as "never defined" when used with the null coalesce operator ($c ?? null). This happened when the code also contained anarray_key_exists()check on the same array parameter after a type-narrowing throw branch.Changes
src/Analyser/MutatingScope.php(Pass 2 offilterBySpecifiedTypes) to skip conditional expressions whose type holder hasNocertainty during supertype matchingtests/PHPStan/Rules/Variables/data/bug-14458.phptestBug14458intests/PHPStan/Rules/Variables/NullCoalesceRuleTest.phpRoot cause
The two-pass conditional expression resolution introduced in #5386 uses a supertype match in Pass 2 as a fallback when exact matching fails. The problem is that the "undefined" branch of a conditional variable (with
Nocertainty) has a broader condition type than the "defined" branch. This makes the supertype check succeed for the "undefined" branch while failing for the "defined" branch, causing the variable to be incorrectly resolved as never defined.For example, with
if (isset($payload['c'])) { $c = ...; }, the conditional expressions are:$payloadhas typearray&hasOffsetValue('a', 'b')&hasOffsetValue('c', ...)->$cis defined (Yes)$payloadhas typearray&hasOffsetValue('a', 'b')->$cis undefined (No)When a later
array_key_exists('cpwv', $payload)narrows$payload's type, Pass 2's supertype check matches condition 2 (broader type is supertype) but not condition 1 (narrower type is not supertype), causing$cto be incorrectly marked as never defined.The fix skips
No-certainty holders in Pass 2, since they represent the "variable is not defined" case which should only be resolved via exact matching (Pass 1). This prevents the asymmetric supertype match from incorrectly resolving conditionally-defined variables as undefined.Test
The regression test reproduces the exact scenario from the issue: a function with
array<mixed>parameter where a throw branch narrows the array type, anisset()check conditionally defines$c, and a subsequentarray_key_exists()check triggers the incorrect resolution. The test expects no errors for$c ?? null.Fixes phpstan/phpstan#14458